ci(object): make the ObjectHeader shape-descriptor census a real gate by proggeramlug · Pull Request #8110 · PerryTS/perry

proggeramlug · 2026-08-14T22:53:49Z

What

Wires scripts/shape_descriptor_census.py into the lint job, and fixes a
vacuous arm inside it that the wiring exposed.

Why

#8086 shipped the exact-callsite census that #8067 required — the instrument
that keeps ObjectHeader::object_type, field_count and keys_array retired
while ShapeId takes over their facts, and the thing #8047 says must exist
before anyone touches the layout. It is a good instrument: it strips comments
and string literals, classifies every site declaration vs access, diffs the
full multiset against a reviewed baseline (so it can tell an ObjectHeader read
from another struct's identically-named field), and carries its own lexer
self-test plus sabotage self-tests.

It is invoked by nothing:

$ grep -rn "shape_descriptor_census" --include=*.yml --include=*.sh \
      --include=*.py --include=*.toml --include=*.md . \
  | grep -v '^./scripts/shape_descriptor_census'
scripts/shape_descriptor_census.py:15:BASELINE_PATH = ROOT / "scripts" / ...

The only hit is the script naming its own baseline. lint runs fifteen other
python audits; this is not one of them.

The vacuous arm

Wiring it up and sabotage-testing it found that the emitted-guard check could
not fail for any function it names. It matched

re.search(r"expected_keys|add\s*\([^\n]*\"(?:0|12|16)\"", body)

but all four functions in that list build their header address with
blk.gep(I8, &p, &[(I64, "N")]), not add(..):

function	actual emission
`emit_class_field_loop_preheader_check`	`blk.gep(I8, &obj_ptr, &[(I64, "4")])`
`emit_proven_shape_recheck`	`blk.gep(I8, &obj_ptr, &[(I64, "8")])`
`emit_class_field_inline_precheck`	`blk.gep(I8, &obj_ptr, &[(I64, "4")])`
`emit_element_shape_field_load`	`blk.gep(I8, &elem_ptr, &[(I64, "8")])`

So the pattern targeted a syntax none of them uses. Planting a read of the
keys_array offset that #8047 removes —
gep(I8, &elem_ptr, &[(I64, "16")]) in emit_element_shape_field_load — left
the census green. This patch matches the gep form as well.

The two write-PIC checks below it are unaffected: lower_generic_property_get
and the proxy_reflect emitters genuinely do use ctx.block().add(I64, .., "N"),
and their own regexes already spell that.

Verification

Sabotage-verified, every change reverted afterwards, clean tree green before and
after each:

sabotage	before	after
new `(*obj).field_count` read in `object/spill.rs`	exit 1	exit 1
keys-pointer token in `proxy/put_value.rs::dyn_ic_try_store`	exit 1	exit 1
offset 16 in `emit_element_shape_field_load`	exit 0	exit 1
offset 12 in `emit_class_field_loop_preheader_check`	exit 0	exit 1
offset 12 in `emit_proven_shape_recheck`	exit 0	exit 1
offset 12 in `emit_class_field_inline_precheck`	exit 0	exit 1

All sixteen lint steps, enumerated from .github/workflows/test.yml rather
than from memory, pass on this branch: cargo fmt --all -- --check,
check_file_size.sh, workspace_architecture.py (--self-test and --check),
local_binding_type_audit.py, gc_store_site_inventory.py,
addr_class_inventory.py, class_id_collisions.py, gc_pin_sites.py,
gc_runtime_root_holders.py, check_gc_env_knobs.py,
check_llvm_corpus_currency.py, check_gc_doc_claims.py, raw_handle_debt.py,
check_test_registration.py, and the newly wired census.

No behaviour change: the census reports the same summary on this branch as on
12f758a22.

Note this is a new gate, so per CLAUDE.md it has never been green — run it
once on main before promoting it to a required context.

Not in this PR

The #8047 layout change itself. A full re-audit on 12f758a22 found the
prerequisite still incomplete; findings are posted on #8047.

Refs #8047, #8067, #8086.

Summary by CodeRabbit

Bug Fixes
- Improved validation for shape-descriptor metadata and emitted offset patterns, including additional generated forms.
- Corrected matching for guard checks and expanded coverage across descriptor-related access patterns.
Tests
- Added a required lint-time census and self-checks to detect retired metadata usage and verify shape identifier authority.
- Updated validation baselines to reflect current source usage and ensure consistent counts.

#8086 built the exact-callsite census #8067 asked for — the instrument that keeps `object_type`, `field_count` and `keys_array` retired as ShapeId takes over their facts — and then wired it into nothing. `grep -rn shape_descriptor_census` over every workflow, script and doc returns only the script naming its own baseline, so it has never been able to fail a build. Add it to `lint` alongside the other fifteen audits. Wiring it up exposed that one of its two arms was vacuous. The emitted-guard check rejected `add(..., "0"|"12"|"16")`, but all four functions in its list — `emit_class_field_loop_preheader_check`, `emit_proven_shape_recheck`, `emit_class_field_inline_precheck` and `emit_element_shape_field_load` — build their header address with `blk.gep(I8, &p, &[(I64, "N")])`. The pattern therefore matched a syntax those functions never emit. Planting `gep(I8, &elem_ptr, &[(I64, "16")])` in `emit_element_shape_field_load` — a read of the `keys_array` offset #8047 removes — left the census green. Match the gep form too. Sabotage-verified, each reverted after: * new `(*obj).field_count` read in `object/spill.rs` -> exit 1 * keys-pointer token in `proxy/put_value.rs::dyn_ic_try_store` -> exit 1 * offset 16 in `emit_element_shape_field_load` -> exit 1 (was 0) * offset 12 in the three `class_field_inline_guard` emitters -> exit 1 each Clean tree green before and after every one. No behaviour change; the census reports the same summary it did on 12f758a. Refs #8047, #8067, #8086.

coderabbitai · 2026-08-14T22:54:03Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: aaf11c2b-0c9d-4d18-835b-a2d237920937

📥 Commits

Reviewing files that changed from the base of the PR and between e75191b and 9bcf2ce.

📒 Files selected for processing (4)

.github/workflows/test.yml
changelog.d/8110-shape-census-gate.md
scripts/shape_descriptor_census.py
scripts/shape_descriptor_census_baseline.json

📝 Walkthrough

Walkthrough

The PR updates the shape-descriptor census to recognize current descriptor and emitter forms, refreshes its baseline, and runs it as a cancellation-safe required lint step.

Changes

Shape descriptor census

Layer / File(s)	Summary
Census validation and baseline `scripts/shape_descriptor_census.py`, `scripts/shape_descriptor_census_baseline.json`	The census accepts qualified `HashMap` and `PtrHashMap` types, detects `add` and `gep` offset calculations, and records updated callsites and totals.
Lint workflow enforcement `.github/workflows/test.yml`, `changelog.d/8110-shape-census-gate.md`	The lint workflow runs the census when the workflow is not cancelled. The changelog records the gate and related validation updates.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

PerryTS/perry#8074: Introduced the shape-descriptor census that this PR extends.
PerryTS/perry#7278: Added a cancellation-safe required validation gate to the lint workflow.
PerryTS/perry#7306: Modified cancellation-aware lint-gate execution in the same workflow.

Suggested labels: tooling

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch gc/8047-census-gate

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

proggeramlug · 2026-08-14T22:59:37Z

Verified and endorsed — and this is my miss to own.

I audited and merged #8086. My checks were: all four lint gates on a landing-equivalent tree, perry-runtime (2351/0), the full perry-codegen suite diffed test-for-test against main's 11-failure baseline, and specifically whether the raw-handle ratchet had been softened to accommodate the new with_{mut,const}_ptr combinator (it had not). All of that was true and none of it was the right question for the census.

Confirmed independently on main at 12f758a22:

$ grep -rn "shape_descriptor_census" .github/workflows/
(no output)

$ git grep -l "shape_descriptor_census" origin/main
scripts/shape_descriptor_census.py

The script is referenced by exactly one file — itself. #8067's acceptance asked for a census; #8086 built it and wired it into nothing, so it has never gated anything and never could.

And the arm that would have mattered was vacuous. Your finding that it matched add(..., "0"|"12"|"16") while all four named functions emit blk.gep(I8, &p, &[(I64, "N")]) means that even once wired, planting a keys_array-offset read in emit_element_shape_field_load left it green — which you demonstrated rather than argued. That is the same failure this repo has hit repeatedly, and it is worth noting it survived because nothing ran it: an unwired gate cannot fail, so its vacuity was undetectable by any amount of it "passing".

The generalisable lesson I am taking from this, since it is a gap in how I audit: when a PR adds a gate, checker or census, verifying its tests pass is not enough — verify it is wired into a workflow, and verify it can go red. "Does this new gate run?" and "can it fail?" are two separate questions and I asked neither.

Six sabotages with the clean tree re-verified green after each, and all 16 lint steps enumerated from the workflow rather than remembered, is the right standard. No objection from me — this should land.

Two things from your report worth carrying into #8112/#8113 rather than losing here: perry-ffi's object_header_matches_runtime has never executed (runtime-link enabled nowhere in CI, and cargo-test is per-package), and perry-ffi is published to crates.io, so an old mirror against a new runtime reads class_id from a deleted slot with no compile error. A size/padding divergence being invisible is precisely the failure mode a header shrink has.

Wiring the census into `lint` exposed two things it could not survive. The authority-surface check hardcoded `descriptors: HashMap<u32, ShapeDescriptor>`. #8157 changed that field to `crate::fast_hash::PtrHashMap` (SipHash on a bare u32 was 25% of self time in `shapes`), so the check failed on a rename it should not care about. The fact it asserts is that a by-id table EXISTS — the pattern now accepts an optional path qualifier and either hasher, and a real sabotage (swapping the field to `Vec<ShapeDescriptor>`) still trips it. The exact-callsite baseline is refreshed for six sites added since it was written: five in `param_type_guard.rs` from #8165's class-typed parameter guards, one in `process/node_module/source_map.rs`. They are raw reads of the three header words #8047/#8113 are trying to retire — recorded rather than hidden, so the migration list stays honest.

…eys_array sites The rebase onto PerryTS#8110 (census as a real gate) and PerryTS#8157 (PtrHashMap shape probes) brought two new `keys_array` callsites that postdate this branch's baseline: `param_type_guard.rs` (PerryTS#8094) and `process/node_module/source_map.rs` (PerryTS#7312). Both are ordinary uses of a field this change keeps; neither reintroduces `object_type` or `field_count`. The addr-class ratchet baseline is restored to main's verbatim: this branch produces the same verdict main does (542 held sites, the same two pre-existing stale entries), so the branch-local regeneration only served to drop PerryTS#7272's provenance comment. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj

…oxy.rs attribution Re-measured against `3be2016c1` (after PerryTS#8157's PtrHashMap shape probes and PerryTS#8110's census gate), quiet M1 mini, best-of-5, instructions AND peak RSS. The PerryTS#8157 hypothesis is REFUTED: 0 of 19 rows are faster and 12 pay more than 1%. The regression is slightly worse than the pre-PerryTS#8157 table on most rows (deeplist +8.20 -> +9.03, retain1 +7.99 -> +8.24, churn +1.76 -> +3.08); only shapes improves. That is what the arm-C partition predicts -- the dominant rows are footprint-coupled, and a cheaper probe cannot recover a cost that is not probing. New: PerryTS#8094 landed after this branch's original base and read both deleted words in param_type_guard::plain_object, so the rebase converts two free u32 loads into two descriptor probes on a path that is the PerryTS#2 self-time symbol on interp. interp/iso_miss/pipeline go from ~+0.3% to +3.35/+2.86/+4.33%. asyncpipe's +2.90% peak RSS is arena block quantization, not a footprint regression: it is exactly one 1 MB block, it flips sign with the nursery cap, and the shrunk arm holds strictly less live data. The proxy.rs:1523 attribution carried by the previous revision is withdrawn. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj

Ralph Küpper added 2 commits August 15, 2026 00:53

docs(changelog): add fragment for #8110

3d6e831

This was referenced Aug 14, 2026

perf(object/GC): finish the common-object header shrink from 56 B to 40 B after shape-transition migration #8047

Open

perf(object): remove the derivable object_type and field_count header words — 56 B -> 48 B #8113

Open

Ralph Küpper added 2 commits August 16, 2026 07:44

Merge remote-tracking branch 'origin/main' into HEAD

c14576d

proggeramlug marked this pull request as ready for review August 16, 2026 05:46

proggeramlug merged commit 3be2016 into main Aug 16, 2026
42 of 55 checks passed

proggeramlug deleted the gc/8047-census-gate branch August 16, 2026 05:46

proggeramlug mentioned this pull request Aug 16, 2026

fix(next): pass production App Route dylib gate #8082

Merged

proggeramlug mentioned this pull request Aug 16, 2026

perf(object): remove the derivable object_type and field_count header words (56 B -> 48 B) [HELD: #8157 refuted; footprint-coupled residual + new #8094 guard cost] #8122

Draft

Navigation Menu

Uh oh!

Uh oh!

ci(object): make the ObjectHeader shape-descriptor census a real gate - #8110

ci(object): make the ObjectHeader shape-descriptor census a real gate#8110
proggeramlug merged 4 commits into
mainfrom
gc/8047-census-gate

proggeramlug commented Aug 14, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 14, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Uh oh!

proggeramlug commented Aug 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

proggeramlug commented Aug 14, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

The vacuous arm

Verification

Not in this PR

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Uh oh!

proggeramlug commented Aug 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

proggeramlug commented Aug 14, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 14, 2026 •

edited

Loading